refactor: replace badge styles import to use badge.css directly#30
refactor: replace badge styles import to use badge.css directly#30javier-godoy merged 6 commits intomasterfrom
Conversation
Update needed to make the component compatible with Vaadin 24 & Vaadin 25.
Version update indicating component updates for Vaadin 24 & Vaadin 25 support.
WalkthroughThis PR resolves a Vaadin 25 build error by replacing Vaadin's global badge module reference with a local TypeScript module and inline CSS. The project version is bumped to 1.2.0-SNAPSHOT, Vaadin dependency is updated to 25.0.2, and test assets are restructured to support version-specific styling configurations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/resources/META-INF/resources/frontend/src/fc-badge-list.ts (1)
118-122: Potential null reference indisconnectedCallback.The code accesses
this.__parentbefore callingsuper.disconnectedCallback(), but then attempts to setparent.resizables = nullafter the super call. If__parentis undefined or null (e.g., if the element was never fully connected), this could throw an error.Suggested defensive fix
disconnectedCallback() { let parent = this.__parent; super.disconnectedCallback(); - parent.resizables = null; + if (parent) { + parent.resizables = null; + } }src/test/java/com/flowingcode/vaadin/addons/badgelist/StyledBadgesDemo.java (1)
114-120: Missing closing parenthesis in CSSvar()function.The padding value is missing the closing parenthesis for the CSS
var()function, which will result in invalid CSS and the padding style not being applied.🐛 Proposed fix
private Icon createIcon(VaadinIcon vaadinIcon) { Icon icon = vaadinIcon.create(); - icon.getStyle().set("padding", "var(--lumo-space-xs"); + icon.getStyle().set("padding", "var(--lumo-space-xs)"); return icon; }
🤖 Fix all issues with AI agents
In `@src/main/resources/META-INF/resources/frontend/src/fc-badge-list.ts`:
- Line 23: The imported CSS string badgeStylesContent is currently used raw in
the component's static styles; wrap it with Lit's unsafeCSS by importing
unsafeCSS from 'lit' and replacing the raw entry in the static styles array with
unsafeCSS(badgeStylesContent), keeping any other css`...` entries intact so
static styles contains CSSResult/Sheet objects only.
🧹 Nitpick comments (3)
src/test/java/com/flowingcode/vaadin/addons/AppShellConfiguratorImpl.java (1)
25-28: Consider addingserialVersionUIDfor theSerializableinterface.
AppShellConfiguratorextendsSerializable, so this class should ideally declare aserialVersionUIDto avoid potential serialization issues. However, since this is a test-only marker class, this is a minor concern.Suggested fix
`@Theme` public class AppShellConfiguratorImpl implements AppShellConfigurator { - + private static final long serialVersionUID = 1L; }src/test/resources/META-INF/frontend/styles/fc-badge-list.css (1)
1-8: Missing license header.This file is missing the Apache 2.0 license header that is present in the other CSS files added in this PR (e.g.,
styled-badges-demo-v25.css). For consistency and compliance, consider adding the standard license header.Also note that the
.custom-styled-badgerule is duplicated instyled-badges-demo-v25.css. If both files are used together, verify this duplication is intentional and doesn't cause specificity conflicts.src/main/resources/META-INF/resources/frontend/styles/badge.css (1)
177-187: RTL selectors use descendant selector inconsistently with LTR counterparts.Lines 118-124 use the direct child combinator (
>) for icon margin adjustments, but the RTL selectors here use a descendant selector (space). This inconsistency could cause the RTL styles to apply to nested icons unintentionally.♻️ Suggested fix for consistency
/* RTL specific styles */ -[dir='rtl'][theme~='badge'] vaadin-icon:first-child { +[dir='rtl'][theme~='badge']>vaadin-icon:first-child { margin-right: -0.375em; margin-left: 0; } -[dir='rtl'][theme~='badge'] vaadin-icon:last-child { +[dir='rtl'][theme~='badge']>vaadin-icon:last-child { margin-left: -0.375em; margin-right: 0; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pom.xmlsrc/main/java/com/flowingcode/vaadin/addons/badgelist/BadgeList.javasrc/main/resources/META-INF/resources/frontend/src/fc-badge-list.tssrc/main/resources/META-INF/resources/frontend/styles/badge.csssrc/test/java/com/flowingcode/vaadin/addons/AppShellConfiguratorImpl.javasrc/test/java/com/flowingcode/vaadin/addons/badgelist/StyledBadgesDemo.javasrc/test/resources/META-INF/frontend/styles/fc-badge-list.csssrc/test/resources/META-INF/frontend/styles/styled-badges-demo-v25.css
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/com/flowingcode/vaadin/addons/badgelist/StyledBadgesDemo.java (6)
src/test/java/com/flowingcode/vaadin/addons/badgelist/BadgeListDemo.java (1)
DemoSource(29-64)src/test/java/com/flowingcode/vaadin/addons/badgelist/ReadOnlyBinderDemo.java (1)
DemoSource(41-116)src/main/java/com/flowingcode/vaadin/addons/badgelist/BadgeList.java (1)
SuppressWarnings(37-94)src/test/java/com/flowingcode/vaadin/addons/badgelist/BadgeListDemoView.java (1)
SuppressWarnings(30-44)src/test/java/com/flowingcode/vaadin/addons/badgelist/BaseBadgeListDemo.java (1)
SuppressWarnings(30-55)src/test/java/com/flowingcode/vaadin/addons/badgelist/DemoView.java (1)
SuppressWarnings(27-35)
🔇 Additional comments (8)
pom.xml (3)
7-7: LGTM!Version bump to 1.2.0-SNAPSHOT is appropriate given the breaking change in styling approach for Vaadin 24/25 compatibility.
473-473: LGTM!The exclusion of
AppShellConfiguratorImpl.classfrom the demo-jar is correct since this is a test-only configuration class that shouldn't be shipped with the demo artifact.
502-511: Thevaadin-devdependency is correctly marked as optional.Vaadin's
vaadin-devis a development-mode-only dependency that provides the frontend dev server, live reloading, and frontend toolchain (Vite/Node/npm/pnpm). It is not required for runtime functionality and should be excluded from production builds. Marking it as<optional>true</optional>is the correct approach for Vaadin 25, ensuring it's available during development but omitted from production artifacts.src/test/resources/META-INF/frontend/styles/styled-badges-demo-v25.css (1)
21-28: LGTM!The CSS rules correctly target the badge list component's shadow parts and custom badge class. The styling is consistent with the demo requirements for Vaadin 25.
src/main/resources/META-INF/resources/frontend/styles/badge.css (2)
1-25: License headers look good.The dual license header structure correctly attributes both Flowing Code and Vaadin for the derived styling work.
26-175: Base badge styling and variants are well-structured.The CSS correctly implements Lumo-themed badge styling with proper use of CSS custom properties. The selectors appropriately use attribute matching for theme variants.
src/main/java/com/flowingcode/vaadin/addons/badgelist/BadgeList.java (1)
27-27: Good approach to resolve Vaadin 25 compatibility.Replacing the global Vaadin badge module reference with local TypeScript and CSS resources is the correct solution for the Vite/Rollup resolution issue reported in
#27. The@CssImportannotation properly includes the new badge.css stylesheet.Also applies to: 38-39
src/test/java/com/flowingcode/vaadin/addons/badgelist/StyledBadgesDemo.java (1)
34-50: Conditional compilation structure for Vaadin version support looks correct.The
DemoSourceannotations andCssImportannotations are properly structured for conditional inclusion based on Vaadin version, supporting both v24 and v25 styling paths.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
2 participants
Update needed to make the component compatible with Vaadin 24 & Vaadin 25.
Close #27
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.